rust: Add a Rust compatible API implementation. #141
Conversation
soburi
changed the title
soburi
force-pushed
the
rust_impl
branch
6 times, most recently
from
7a556b5 to
f86a047
Compare
soburi
changed the title
soburi
requested review from
DhruvaG2000,
beriberikix and
szczys
as code owners
pillo79
left a comment
pillo79
left a comment
There was a problem hiding this comment.
Awesome, looking forward to see this grow! 🥳 🤗
Kconfig
Outdated
| imply CBPRINTF_FP_SUPPORT | ||
| imply RING_BUFFER | ||
| select UART_INTERRUPT_DRIVEN | ||
| select RUST |
There was a problem hiding this comment.
I think this should currently be an optional feature, to avoid adding the strong dependency right away.
.github/workflows/build.yml
Outdated
| run: | | ||
| git clone https://github.com/arduino/ArduinoCore-API.git $MODULE_PATH/../ArduinoCore-API | ||
| cp -rfp $MODULE_PATH/../ArduinoCore-API/api $MODULE_PATH/cores/arduino/ | ||
| apt install rustup |
There was a problem hiding this comment.
The recommended way to install Rust is via the rustup.rs installer script, Ubuntu's rustup package from apt is often outdated and may not work correctly
There was a problem hiding this comment.
Also,
All initialization is lumped into one "Initialize" step mixing unrelated concerns (cloning API, copying files, Rust setup)...
Better structure:
- name: Clone ArduinoCore-API
[...]
- name: Install Rust toolchain
[...]
There was a problem hiding this comment.
Fixed. Thank you for your comment.
There was a problem hiding this comment.
@soburi you didn't address the first comment, why are we not using curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh instead of apt?
There was a problem hiding this comment.
I missed it. It was addressed.
.github/workflows/build.yml
Outdated
| rustup target add thumbv6m-none-eabi | ||
| rustup target add thumbv7em-none-eabihf | ||
| rustup target add thumbv7em-none-eabi | ||
| rustup target add thumbv7m-none-eabi |
There was a problem hiding this comment.
Installing targets one-by-one is slower. Rustup supports installing multiple targets in one command
.github/workflows/build.yml
Outdated
| rustup target add thumbv6m-none-eabi | ||
| rustup target add thumbv7em-none-eabihf | ||
| rustup target add thumbv7em-none-eabi | ||
| rustup target add thumbv7m-none-eabi |
There was a problem hiding this comment.
| rustup target add thumbv7m-none-eabi | |
| rustup target add thumbv6m-none-eabi thumbv7em-none-eabihf thumbv7em-none-eabi thumbv7m-none-eabi |
We will gradually add a Rust compatible API implementation conforming to Apache-2. This PR adds an alternative implementation of Common.cpp. Signed-off-by: TOKITA Hiroshi <[email protected]>
soburi
force-pushed
the
rust_impl
branch
6 times, most recently
from
a6ef806 to
ba14701
Compare
Adding Rust architecture modules for ensure compilation. Signed-off-by: TOKITA Hiroshi <[email protected]>
|
@pillo79 can you please take another look if you still have pending comments? |
pillo79
left a comment
pillo79
left a comment
There was a problem hiding this comment.
Sorry, I might be missing something huge, but... it still seems to me that effectively Rust is being forced on every core use. What is gating the new add_custom_command in the main CMakeLists.txt?
Again, I think it's awesome to see this working 😍, but IMO it should be optional at this stage.
|
|
||
| if ARDUINO_API | ||
|
|
||
| config USE_ARDUINO_API_RUST_IMPLEMENTATION |
There was a problem hiding this comment.
I am missing where this symbol is effectively used? AFAICS CMakeLists.txt will try to invoke cargo build regardless of this being set or not.
3 participants
We will gradually add a Rust compatible API implementation conforming to Apache-2.
This PR adds an alternative implementation of Common.cpp.